adds object/function filter to local plugin#807
Conversation
Reimplements 920d90b
| console.log(e.target.value); | ||
| const filterAsMap = new Map(); | ||
| filterAsMap.set('name', e.target.value); | ||
| this.props.setFilter(filterAsMap); |
There was a problem hiding this comment.
@arnimhanke @benoj it seems wrong that the internal use of ImmutableJS is leaking out here. Thoughts on adding a toJSON() to the reducer?
There was a problem hiding this comment.
Agreed. TBH I'm not a huge fan of Immutable.JS so would be good not to have that bleed into outside projects using Griddle
There was a problem hiding this comment.
would be good not to have that bleed into outside projects using Griddle
Agreed. I'm not thrilled with the places that it does leak, but it seems likely to be a nontrivial performance hit to un-Immutable data before exposing rows to filter functions (for example).
There was a problem hiding this comment.
Yes - This is probably the reason I left it there originally (It was a while back I started this...)
Might be safe to leave it as is for now for perf. reasons and look to add a feature where can optionally un-Immutable data before filtering?
There was a problem hiding this comment.
e.g. setFilter(filter, { json: true }) where json means dont sent me immutableJs object
There was a problem hiding this comment.
I agree with you two!
But I must admit that I don't know exactly which part of the code you want to be changed, so i cant give any input in this discussion :-/
There was a problem hiding this comment.
Added some TypeScript smarts, too.
There was a problem hiding this comment.
As far as i can see it looks good :)
Is there something I can do for progress?
|
I'll try to ship a point release with this yet today. Thanks @benoj and @arnimhanke! |
|
This has been released with cc @waclock |
|
Do you mean 1.13.0 (I cant find the changes in 1.3.0) ? :) |
That's what I said 😉 |
4 participants
Git fail. This all belongs to #747 to close #746.